Skip to content

Refactor code to move the current logic to search for WorkerConfigs to a default worker configuration resolver #11229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 33 commits into
base: dev
Choose a base branch
from

Conversation

surgupta-msft
Copy link
Contributor

Issue describing the changes in this PR

resolves #11218

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
    • Otherwise: Link to backporting PR
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

Refactor code to move the current logic to search for WorkerConfigs to a default worker configuration resolver. The current logic finds workerConfigs by scanning workers directory within the Host. This resolver should use Options pattern to get the required input values.

These changes should make it easier to add a new Resolver in future which will resolve workerConfigs from specified worker probing paths.

@Copilot Copilot AI review requested due to automatic review settings August 1, 2025 19:04
@surgupta-msft surgupta-msft requested a review from a team as a code owner August 1, 2025 19:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the worker configuration resolution logic by extracting it from RpcWorkerConfigFactory into a dedicated DefaultWorkerConfigurationResolver class. The refactoring introduces the Options pattern for configuration management and enables easier extension with additional resolvers in the future.

Key changes include:

  • Extract worker configuration discovery logic into a new resolver pattern
  • Implement Options pattern for worker configuration resolver settings
  • Update dependency injection to support the new resolver architecture

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
IWorkerConfigurationResolver.cs New interface defining worker configuration resolution contract
DefaultWorkerConfigurationResolver.cs New implementation that handles worker config path discovery from workers directory
WorkerConfigurationResolverOptions.cs Options class for resolver configuration with JSON serialization support
WorkerConfigurationResolverOptionsSetup.cs Setup class for configuring resolver options using Options pattern
RpcWorkerConfigFactory.cs Refactored to use the new resolver instead of inline logic
Various test files Updated to accommodate new dependency injection requirements

@surgupta-msft surgupta-msft requested a review from kshyju August 4, 2025 16:12
@surgupta-msft surgupta-msft requested review from kshyju and jviau August 6, 2025 19:27
@surgupta-msft surgupta-msft requested a review from kshyju August 12, 2025 15:20
@surgupta-msft surgupta-msft requested a review from brettsam August 13, 2025 05:55
Copy link
Member

@brettsam brettsam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple nits about internal methods that we can skip for now, but I wanted to capture them. They've been around a while so nothing was introduced here but they bugged me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor code to move the current logic to search for WorkerConfigs to a default worker configuration resolver
5 participants